Skip to content

Fixes #30117: hide soft-deleted owners on the Test Suite detail page - #30520

Merged
mohityadav766 merged 11 commits into
mainfrom
investigate-issue-30117
Jul 29, 2026
Merged

Fixes #30117: hide soft-deleted owners on the Test Suite detail page#30520
mohityadav766 merged 11 commits into
mainfrom
investigate-issue-30117

Conversation

@mohityadav766

@mohityadav766 mohityadav766 commented Jul 27, 2026

Copy link
Copy Markdown
Member

Describe your changes:

Fixes #30117

Editing owners on a logical Test Suite failed with array item index is out of range when an owner was soft-deleted. The Test Suite detail page surfaced the soft-deleted owner (unlike the 17+ data-asset pages, which fetch owners as non-deleted) and then computed a positional JSON Patch against a longer array than the server's NON_DELETED PATCH base.

Two small, consistent changes make the Test Suite page behave like every other detail page:

  • FrontendgetTestSuiteByName defaults includeRelations: 'owners:non-deleted,experts:non-deleted' (the convention the data-asset APIs already use).
  • BackendTestSuiteResource.getByName now accepts includeRelations and passes it to getByNameInternal. It previously ignored the param (unlike getById and TableResource.getByName), which made the frontend change a no-op.

No PATCH-base change — the soft-deleted owner is simply hidden and the owner-edit diff stays aligned with the server.

Type of change:

  • Bug fix

High-level design:

A single server-side PATCH base can't align with both owner representations at once (some clients diff against owners:all, most against owners:non-deleted) unless the PATCH declares the base it used — a larger, representation-aware change the issue flags as a design call. The reported crash is just the Test Suite page being inconsistent with every other owner-editing page; making it consistent fixes the crash with no regression. Showing/removing soft-deleted owners rather than hiding them remains the issue's separate design call.

An earlier iteration changed the server PATCH base to owners=ALL. That fixed Test Suite but regressed the 17+ owners:non-deleted pages (a positional owner remove hit the wrong owner against the longer base). Reverted in favor of this consistency fix.

Tests:

Use cases covered

  • The Test Suite getByName endpoint honors includeRelations=owners:non-deleted and hides a soft-deleted owner (while include=all still surfaces it).
  • getTestSuiteByName defaults to owners:non-deleted,experts:non-deleted and respects a caller override.

Unit tests

  • openmetadata-ui/.../rest/testAPI.test.tsgetTestSuiteByName param default + override.

Backend integration tests

  • openmetadata-integration-tests/.../TestSuiteResourceIT.javaget_byName_honorsIncludeRelations_hidesSoftDeletedOwner. Verified RED (without the endpoint wiring the soft-deleted owner is still returned) and GREEN.

Playwright (UI) tests

  • Not applicable — covered by the unit + integration tests above.

Manual testing performed

Ran the new IT against the embedded app (MySQL + Elasticsearch): RED without the getByName wiring, GREEN with it. Frontend jest (getTestSuiteByName, 3 pass) and UI checkstyle clean.

UI screen recording / screenshots:

Not applicable — no layout change. A soft-deleted owner no longer appears in the Test Suite owner list, matching every other detail page.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests and listed them above.
  • I have added a test that covers the exact scenario we are fixing.

🤖 Generated with Claude Code

Greptile Summary

This PR aligns Test Suite owner editing with the server’s non-deleted PATCH representation.

  • Adds includeRelations support to the Test Suite get-by-name endpoint.
  • Defaults UI Test Suite fetches to non-deleted owners and experts while preserving explicit caller overrides.
  • Adds frontend unit coverage and backend integration coverage for relation filtering.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java Forwards the per-relation inclusion query to the shared get-by-name implementation.
openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts Defaults Test Suite owners and experts to non-deleted while retaining explicit overrides.
openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts Covers both the new relation default and caller-provided inclusion behavior.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java Verifies get-by-name filters a soft-deleted owner when requested.

Sequence Diagram

sequenceDiagram
  participant UI as Test Suite UI
  participant API as getTestSuiteByName
  participant Service as TestSuiteResource
  participant Repo as Repository
  UI->>API: Fetch suite
  API->>Service: GET by name with owners/experts non-deleted
  Service->>Repo: Resolve fields with per-relation includes
  Repo-->>UI: Filtered TestSuite representation
  UI->>UI: Compute positional owner JSON Patch
  UI->>Repo: PATCH against non-deleted base
  Repo-->>UI: Updated TestSuite
Loading

Reviews (10): Last reviewed commit: "Fixes #30117: honor includeRelations on ..." | Re-trigger Greptile

Editing owners failed with "array item index out of range" when an owner
user was soft-deleted. Detail pages fetch with include=all (soft-deleted
owner present) and compute a positional JSON Patch, but the server loaded
the PATCH base with NON_DELETED, which dropped that owner and shrank the
array so the positional op referenced an index past the end. The dangling
ownership also could not be removed.

Both patch() entry points now load the original with a RelationIncludes
that keeps the entity at NON_DELETED but resolves owners/experts/reviewers/
followers with ALL, so the positional patch stays in range and the ID-based
updateOwners diff removes the dangling OWNS row. Adds two integration tests
reproducing the reported crash (deleted owner sorting last) and the silent
wrong-owner removal (deleted owner in the middle).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mohityadav766
mohityadav766 requested a review from a team as a code owner July 27, 2026 11:46
Copilot AI review requested due to automatic review settings July 27, 2026 11:46
@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jul 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a backend PATCH failure triggered when an entity has soft-deleted user relationships (owners/experts/reviewers/followers) and the client computes a positional JSON Patch from an include=all payload.

Changes:

  • Update EntityRepository.patch() to load the PATCH “original” with relation-specific includes so soft-deleted user references are present when applying positional JSON Patch ops.
  • Add integration tests to reproduce and verify removal of soft-deleted owners (both “sorts last” crash case and “sorts middle” wrong-index removal case).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java Changes PATCH base-load include behavior for specific user relation fields to prevent positional JSON Patch index errors when soft-deleted user refs exist.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java Adds integration coverage for PATCH removal of soft-deleted owners in two ordering scenarios.
Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4338

  • Same concern as the by-id PATCH path: loading the base with PATCH_ORIGINAL_INCLUDES for every PATCH can cause PATCHes that don't touch owners/experts/reviewers/followers to fail when one of those relations points at a soft-deleted user, because later validation resolves owners with Include.NON_DELETED and throws.

Gate PATCH_ORIGINAL_INCLUDES behind whether the patch actually modifies one of those fields; otherwise keep NON_DELETED behavior to avoid new failures.

    // Get only the fields relevant to this patch operation
    T original;
    try (var ignored = phase("patchLoadOriginalByName")) {
      original = getByName(null, fqn, patchFields, PATCH_ORIGINAL_INCLUDES, false);

Comment on lines +4274 to +4276
T original;
try (var ignored = phase("patchLoadOriginal")) {
original = get(null, id, patchFields, NON_DELETED, false);
original = get(null, id, patchFields, PATCH_ORIGINAL_INCLUDES, false);
Copilot AI review requested due to automatic review settings July 27, 2026 11:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:445

  • Changing PATCH to load the original with owners/experts/reviewers/followers = Include.ALL fixes clients that diff against an include=all representation, but it can break clients that build a positional JSON Patch from a NON_DELETED owner array (they will now apply indices against a longer server-side base and can remove the wrong owner).

This is not just theoretical: the UI’s updateEntityField uses fast-json-patch positional diffs, and some entity GETs explicitly request includeRelations: 'owners:non-deleted,…' (e.g. openmetadata-ui/src/main/resources/ui/src/rest/tableAPI.ts:65-77). In a scenario where a soft-deleted owner sorts before an active owner, a client removing /owners/0 from its filtered array would cause the server to remove the soft-deleted owner instead, leaving the intended active owner unchanged.

Consider a contract-level fix: e.g. allow PATCH to accept an includeRelations/relationIncludes parameter (or header) so the client can declare which representation it diffed against, or switch the UI to a non-positional update for these arrays (whole-array replace / ID-based operations).

  // A PATCH's base ("original") must resolve user references the same way the client did when it
  // computed the diff. Detail pages fetch with include=all, so a soft-deleted owner (or expert,
  // reviewer, follower) is in the client's base. Loading the server base with NON_DELETED drops it
  // and shrinks the array, so a positional JSON-Patch op goes out of range ("array item index out
  // of range") and the dangling relationship can never be removed. Resolve these relations with ALL
  // for the patch base; the entity itself still loads NON_DELETED so a soft-deleted entity is not
  // patched through this path. See issue #30117.
  private static final RelationIncludes PATCH_ORIGINAL_INCLUDES =
      new RelationIncludes(
          NON_DELETED,
          Map.of(
              FIELD_OWNERS, ALL,
              FIELD_EXPERTS, ALL,
              FIELD_REVIEWERS, ALL,
              FIELD_FOLLOWERS, ALL));

Comment on lines +392 to +400
// ===================================================================
// SOFT-DELETED OWNER PATCH TESTS (issue #30117)
//
// A detail page loads the entity with include=all, so a soft-deleted owner is present in the
// client's owner array and in the positional JSON Patch it computes. The server used to load the
// PATCH base with NON_DELETED, which dropped that owner and shrank the array — a positional op
// then referenced an index past the end ("array item index out of range") and the dangling
// ownership could never be removed. These tests reproduce that exact client behavior.
// ===================================================================
Copilot AI review requested due to automatic review settings July 27, 2026 12:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 28, 2026 13:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:443

  • The new PATCH_ORIGINAL_INCLUDES block comment has broken wrapping (standalone "a", "entity", "latent" lines), which makes it harder to read and looks like an accidental formatter artifact. Please reflow it into complete sentences on each line.
  // A PATCH's base ("original") must resolve OWNER references the same way the client diffed
  // against. Detail pages fetch with include=all, so a soft-deleted owner is in the client's base;
  // a
  // NON_DELETED server base drops it and shrinks the array, so a positional JSON-Patch op goes out
  // of range ("array item index out of range") and the dangling ownership can never be removed.

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java:868

  • This test uses Assumptions.assumeTrue(...) to verify conditions that should be deterministic (owners were just assigned and fetched with include=all). If this ever fails it indicates a real regression and should fail the test rather than silently skip it.
    JsonNode ownersBefore = ownersWithIncludeAll(entityPath);
    int doomedIndex = indexOfOwner(ownersBefore, doomedOwner.getId());
    Assumptions.assumeTrue(
        indexOfOwner(ownersBefore, activeOwner.getId()) >= 0 && doomedIndex >= 0,
        "both explicit owners must be assigned and the soft-deleted one surfaced by include=all");

… data assets)

Revert the server-side PATCH-base change. Loading the base with owners=ALL
fixed the Test Suite crash but regressed the 17+ data-asset detail pages that
fetch owners with includeRelations=owners:non-deleted: a positional owner
remove computed against the filtered array removed the wrong owner when applied
to the longer ALL base. A single server base cannot align with both client
owner representations, so the patch itself must carry it - a separate design
call the issue flags.

Fix the reported crash where the divergence actually is: the Test Suite detail
page was the only owner-editing page fetching owners with include=all and no
includeRelations override. getTestSuiteByName now defaults owners/experts to
non-deleted like every data-asset page, so a soft-deleted owner is not surfaced
and the owner-edit diff stays aligned with the server's NON_DELETED PATCH base -
no "array item index out of range". Showing and removing soft-deleted owners
remains the issue's separate design call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts:421

  • The new default includeRelations + inline comment implements a UI-side workaround by filtering soft-deleted owners out of the GET response (to match a NON_DELETED PATCH base). This conflicts with the PR title/description, which describe a server-side fix that keeps soft-deleted owners in the PATCH base so they can be removed. Please reconcile by either (a) including the described backend change and adjusting this default accordingly, or (b) updating the PR title/description to reflect that this change intentionally hides soft-deleted owners instead of preserving them for removal.
      // Resolve owners/experts as non-deleted (consistent with data-asset detail pages), so a
      // soft-deleted owner is not surfaced and the owner-edit diff stays aligned with the server's
      // NON_DELETED PATCH base - otherwise a positional patch throws "array item index out of
      // range". See issue #30117.
      params: {

openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts:886

  • This test case bakes in the new contract that getTestSuiteByName hides soft-deleted owners by default. That seems inconsistent with the PR title/description (server-side PATCH base fix to keep soft-deleted owners present so they can be removed). Please confirm the intended end-state and adjust either the implementation/tests or the PR metadata accordingly.
      it('should default owners/experts to non-deleted so soft-deleted owners stay hidden (issue #30117)', async () => {

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts:890

  • These tests only assert that the client sends an includeRelations query param. However, the current backend GET /dataQuality/testSuites/name/{name} endpoint does not accept includeRelations, so this test can pass even if the runtime behavior (hiding soft-deleted owners and avoiding the PATCH crash) is unchanged.

Consider adding coverage that exercises the end-to-end behavior after the backend endpoint supports includeRelations, or adjust the fix to use an endpoint/query param combination that is actually honored by the server today.

      it('should default owners/experts to non-deleted so soft-deleted owners stay hidden (issue #30117)', async () => {
        const mockGet = jest.fn().mockResolvedValue({ data: mockTestSuite });
        jest.mock('./index', () => ({
          __esModule: true,
          default: {

Comment on lines +421 to +425
params: {
...params,
includeRelations:
params?.includeRelations ?? 'owners:non-deleted,experts:non-deleted',
},
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.79% (76987/117008) 49.65% (46269/93187) 50.89% (13932/27375)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

siddhant1
siddhant1 previously approved these changes Jul 29, 2026
…d fix was a no-op)

getTestSuiteByName sends includeRelations=owners:non-deleted, but the Test
Suite getByName endpoint - unlike getById and every other entity's getByName
(e.g. TableResource) - did not accept the param, so the server still resolved
owners with include=all and surfaced the soft-deleted owner. The owner-edit
crash therefore remained.

Wire includeRelations through getByName (same pattern as getById /
TableResource.getByName), and add an IT that proves it: an include=all fetch
still surfaces the soft-deleted owner, but includeRelations=owners:non-deleted
hides it. Verified RED (without the wiring the assertion fails - owner still
surfaced) and GREEN.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java:439

  • The PR description states "No backend change", but this change updates the TestSuite REST API surface by adding/forwarding the includeRelations query parameter on the GET /dataQuality/testSuites/name/{name} endpoint (and adds an IT to cover it). Please update the PR description to reflect that there is a (backward-compatible) backend wiring/API-doc change, so reviewers/release notes don’t miss it.
                      + "If not specified for a field, uses the entity's include value.",
              schema = @Schema(type = "string", example = "owners:non-deleted,followers:all"))
          @QueryParam("includeRelations")
          String includeRelations) {
    return getByNameInternal(

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

@gitar-bot

gitar-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Updates getTestSuiteByName to default owners and experts to non-deleted relations, aligning the Test Suite detail page with other data assets and preventing out-of-range patch errors. No issues found.

✅ 2 resolved
Edge Case: PATCH base=ALL misaligns positional patches from NON_DELETED clients

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:438-445 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4276 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4338
Resolving the PATCH base owners/experts/reviewers/followers with Include.ALL aligns the server base with detail-page clients (include=all), but inverts the alignment for any caller that computed its positional JSON-Patch against a NON_DELETED view (SDK/API consumers or internal code that builds a patch via JsonUtils.getJsonPatch from a NON_DELETED-loaded entity). When soft-deleted owners exist, such a client's array is shorter than the new ALL base, so a positional remove/replace op can now hit the wrong index or go out of range in the opposite direction. This only manifests when soft-deleted related entities are present and is the tradeoff the PR acknowledges as deferred, so impact is low — but callers that diff against NON_DELETED should be confirmed to use include=all, or the patch should be applied against the base the client actually used (e.g. via ETag/If-Match to reject stale bases).

Edge Case: Retained PATCH owners bypass validateOwners, risking sort NPE

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:7869-7883
In getValidatedOwnersForPatch, owners already present in originalOwners are copied straight from the client's patch input into result without going through validateOwners, then the combined list is sorted via Comparator.comparing(EntityReference::getName). Unlike getValidatedOwners (which repopulates name/type/fqn from the DB before sorting), a client that sends a minimal owner reference (id+type only, no name) for an already-assigned owner will produce a null name, and the sort will throw a NullPointerException (HTTP 500). It also means retained owners are returned with whatever metadata the client supplied rather than authoritative DB values. Consider repopulating retained owners' fields (e.g. resolve each retained id via getEntityReferenceById with Include.ALL) or at minimum sorting with a null-safe comparator.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editing owners fails with "array item index out of range" when an owner user is soft-deleted

4 participants